feat: Support negative numbers in LogQL #13091
Conversation
benclive
changed the title
There was a problem hiding this comment.
@cyriltovena I've rethought my approach and moved the changes to the Parser and out of the Lexer. I've added comments on the relevant pieces, and added tests to clarify & valid the expected behaviour.
| | IDENTIFIER NEQ NUMBER { $$ = log.NewNumericLabelFilter(log.LabelFilterNotEqual, $1, mustNewFloat($3))} | ||
| | IDENTIFIER EQ NUMBER { $$ = log.NewNumericLabelFilter(log.LabelFilterEqual, $1, mustNewFloat($3))} | ||
| | IDENTIFIER CMP_EQ NUMBER { $$ = log.NewNumericLabelFilter(log.LabelFilterEqual, $1, mustNewFloat($3))} | ||
| IDENTIFIER GT literalExpr { $$ = log.NewNumericLabelFilter(log.LabelFilterGreaterThan, $1, $3.Val)} |
There was a problem hiding this comment.
This is the crux of new approach: numberFilters now accept literal expressions instead of the NUMBER token. This means the filter can include +1, 1 or -1 as equally valid. They get parsed as a float as other literals do, the value is then extracted via Val.
| @@ -95,6 +95,19 @@ func TestLex(t *testing.T) { | |||
| {`{foo="bar"} | logfmt --strict code"`, []int{OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, PIPE, LOGFMT, PARSER_FLAG, IDENTIFIER}}, | |||
| {`{foo="bar"} | logfmt --keep-empty --strict code="response.code", IPAddress="host"`, []int{OPEN_BRACE, IDENTIFIER, EQ, STRING, CLOSE_BRACE, PIPE, LOGFMT, PARSER_FLAG, PARSER_FLAG, IDENTIFIER, EQ, STRING, COMMA, IDENTIFIER, EQ, STRING}}, | |||
| {`decolorize`, []int{DECOLORIZE}}, | |||
| {`123`, []int{NUMBER}}, | |||
| {`-123`, []int{SUB, NUMBER}}, | |||
There was a problem hiding this comment.
I reverted the changes in the lexer so all negative numbers now evaluate as SUB, NUMBER. The AST does the work now.
| @@ -499,19 +499,19 @@ var ParseTestCases = []struct { | |||
| // label filter for ip-matcher | |||
| { | |||
| in: `{ foo = "bar" }|logfmt|addr>=ip("1.2.3.4")`, | |||
| err: logqlmodel.NewParseError("syntax error: unexpected ip, expecting BYTES or NUMBER or DURATION", 1, 30), | |||
| err: logqlmodel.NewParseError("syntax error: unexpected ip", 1, 30), | |||
There was a problem hiding this comment.
This is the biggest caveat with this change: The error messages presented to users are less clear. Presumably because numberFilter is now an expression, so there is no specific TOKEN to look for next.
There is no change to the syntax, just in the error message.
What this PR does / why we need it:
Adds negative number support to the LogQL
lexerparser.